fix(engine): stop passing -avoid_negative_ts make_zero in muxVideoWithAudio - #3488
fix(engine): stop passing -avoid_negative_ts make_zero in muxVideoWithAudio#3488mincua wants to merge 1 commit into
Conversation
`muxVideoWithAudio` passed `-avoid_negative_ts make_zero` unless the caller
set `preserveAudioPrimingEditList`. In practice the dominant path is an AAC
sidecar copied into mp4, where that flag is actively harmful: ffmpeg's
default is `auto`, which the mp4/mov muxers (AVFMT_TS_NEGATIVE) already
resolve to `disabled`. Forcing `make_zero` overrides the correct default,
discards the priming edit list the sidecar encode created, shifts the video
start_time forward by one AAC frame and writes an empty video edit at t=0 —
which edit-list-honoring players (QuickTime/Safari) render as a black first
frame.
Verified with ffprobe on a copy mux of a 30fps h264 mp4 and an AAC sidecar:
with `make_zero` video start_time 0.066000, elst: [media time -1,
dur 5940] + [media time 6000, dur 180000]
audio start_time 0.042993, elst: [media time -1, ...]
without (this fix) video start_time 0.000000, elst: [media time 6000,
dur 180000]
audio start_time 0.000000, elst: [media time 1024, ...]
The empty leading edit and the offset both disappear, and the audio keeps
its 1024-sample priming edit.
The flag is now never passed for a mux, in any mode. `preserveAudioPrimingEditList`
is part of the exported engine API, so it stays on `MuxVideoWithAudioOptions`
as `@deprecated` and no-op rather than being removed; the two internal callers
that set it (`assembleStage`, distributed `assemble`) drop it.
`buildEncoderArgs` and `streamingEncoder` still pass the flag for video-only
output and are deliberately left alone — those chunks are consumed as
intermediates, not as a delivered mp4/mov.
Fixes heygen-com#3487
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
miga-heygen
left a comment
There was a problem hiding this comment.
R1 at a4bea52 — APPROVE (no merge action from me)
Root cause analysis is correct. For mp4/mov containers, ffmpeg's auto mode for -avoid_negative_ts already resolves to disabled (the mp4/mov muxers set AVFMT_TS_NEGATIVE). Passing make_zero explicitly overrides that default and, on the audio-copy path, discards the AAC priming edit list the sidecar encode created — shifting the video start_time forward by one AAC frame (~21ms) and writing an empty video edit at t=0, which edit-list-honoring players (QuickTime/Safari) render as a black first frame.
Verified clean across all mux paths:
-
Video-only chunk encoders (
buildEncoderArgsinchunkEncoder.ts:416,buildStreamingArgsinstreamingEncoder.ts:372) correctly retainmake_zero— those produce raw elementary output consumed by the concat/mux pipeline, not delivered mp4/mov containers. -
WebM muxing — already skipped
make_zerobefore this PR (the old conditional was guarded by!isWebm). The new container-agnostic assertion atchunkEncoder.test.ts:630now covers all formats uniformly. -
Audio re-encode path (non-copy, when original was not AAC) — previously the only path that set
preserveAudioPrimingEditList: true. Removingmake_zerounconditionally is still correct here: the M4A sidecar's re-encoder also creates a priming edit list, so the fix preserves it for both copy and re-encode. -
All callers of
muxVideoWithAudioupdated:assembleStage.ts(in-process render) anddistributed/assemble.ts. Both verified — no other production call sites exist (checked via grep).
preserveAudioPrimingEditList deprecation — clean pattern: @deprecated JSDoc, option kept on the interface for source compatibility, value no longer read. All production callers already stop passing it in this PR.
Test coverage is strong:
- Negative assertion on
-avoid_negative_tsfor every container format (mp4, mov, WebM — line 630) - Renamed test cases clearly document the regression narrative
- Deprecated-option test verifies no-op behavior
One non-blocking note:
streamingEncoder.ts:370-371 references "See chunkEncoder for the full explanation; same playback compatibility class." The chunkEncoder comment has now changed from explaining the flag's use to explaining its removal in the mux path. The cross-reference could confuse a future reader who follows it from the streaming encoder (which still uses the flag) and reads about why the mux path removed it. Consider updating the streamingEncoder comment to be self-contained: "Raw chunk output is not a delivered container — make_zero is safe here (no edit list to discard)."
— Miga
Fixes #3487
What
muxVideoWithAudiono longer passes-avoid_negative_ts make_zero— in any mode, for any container. One flag removed from the mux path; thepreserveAudioPrimingEditListoption that used to gate it is deprecated and ignored rather than deleted.Why
The flag is actively harmful on the dominant mux path (an AAC sidecar copied into mp4), and it is not needed on any of the others.
The defect. ffmpeg's default is
-avoid_negative_ts auto, i.e. "enabled when required by the target format". The mp4/mov muxers setAVFMT_TS_NEGATIVE(libavformat/movenc.c), soautoalready resolves to disabled for the containers this function writes — mp4 expresses an offset start natively through an edit list. Passingmake_zeroexplicitly overrides that deliberate per-format opt-out. On the copy path it discards the AAC priming edit list the sidecar encode created, shifts the copied video forward by one AAC frame (~21 ms), and makes the muxer write an empty video edit at t=0 (media time: -1). Players and thumbnailers that honor edit lists — QuickTime, Safari, WhatsApp previews,requestVideoFrameCallback— find no video sample at t=0 and render a black first frame.The regression chain. The flag was added in v0.4.45 as belt-and-suspenders against negative DTS, a problem the same commit had already fixed at the source with
-bf 0on the encoder. #1615 (v0.6.116) diagnosed the black-first-frame symptom and fixed it by dropping a second AAC encode. Then:config?.preserveAudioPrimingEditList === true, so the copy path — dominant, because the mixer already pads every input to the exact composition duration — ranmake_zeroagain. Harmless at the time: the mixer wrote raw ADTSaudio.aac, which has no edit list to destroy.audio.aactoaudio.m4a, precisely so the container would carry the priming edit list — but the v0.7.72 gate was not revisited. The callers still passedpreserveAudioPrimingEditList: operation !== "copy", somake_zeroran on virtually every render and destroyed the very edit list fix(producer): mix audio into a container that can record encoder delay #3200 had introduced.Why omitting the flag is correct rather than merely convenient. For mp4/mov, omitting it is the flag's own documented behavior —
auto→ disabled — so this restores ffmpeg's per-format default instead of imposing a new policy. Genuine leading negative DTS is still handled: the priming interval is carried by the audio track's edit list, and the encoder keeps-bf 0, which removes negative DTS at the source.make_zeroalso isn't idempotent — every re-encode pass through it stacks another priming interval (our delivery upscale turned 21 ms into 42 ms, a full frame at 24 fps). And the scenario it was guarding against does not materialize: muxing B-frame video with real leading negative DTS into mp4 comes out correct with the default and broken withmake_zero.How
packages/engine/src/services/chunkEncoder.ts— drop thecopiesContainerizedAaccondition and theargs.push("-avoid_negative_ts", "make_zero")it guarded. Replaced by a comment recording why the flag must not be reintroduced here, so the next person reading the file doesn't re-derive it as a missing safety net.MuxVideoWithAudioOptions.preserveAudioPrimingEditListis part of the exported engine API, so it stays, marked@deprecatedand documented as a no-op, rather than being removed. No breaking change for external callers; removal can happen in a future major.packages/producer/.../render/stages/assembleStage.tsandpackages/producer/.../distributed/assemble.ts— the two internal callers stop passing the option. Indistributed/assemble.tsthenormalizedAudioobject existed only to carry that boolean alongside the path, so it collapses back to a plainnormalizedAudioPath: string | null."keeps negative-timestamp repair for an M4A without a known priming edit list", asserting the flag is passed) is inverted into"never repairs negative timestamps for an M4A sidecar (regression #3487)". Anot.toContain("-avoid_negative_ts")assertion is added to the re-encode case and to the per-container loop, so the flag can't come back for any container without a test failing.Deliberately left alone:
buildEncoderArgsandstreamingEncoderstill pass-avoid_negative_ts make_zerofor video-only output. Those chunks are intermediates consumed by the concat/mux stages, not delivered mp4/mov, they carry no audio priming edit list to destroy, and the flag's original negative-DTS rationale still applies there. Changing them is out of scope for this fix and would need its own evidence.Test plan
1. Caller-shaped ffmpeg repro — a 30 fps h264 mp4 plus a 48 kHz AAC
.m4asidecar, muxed with exactly the argvmuxVideoWithAudiobuilds, with and without the flag.ffprobe:make_zero(before)start_time0.0210290.000000elstmedia time: -1, dur 323+media time: 0, dur 30720media time: 0, dur 30720elstmedia time: 0(priming trim destroyed)media time: 1024(priming preserved)duration2.0213332.000000The empty leading video edit and the offset both disappear, and the audio keeps its 1024-sample priming edit.
2. Minimal repro from the issue (24 fps, both 44.1 kHz and 48 kHz) —
start_time0.023031/0.020996with the flag,0.000000without; themedia time: -1empty edit is present in each broken case and absent in each fixed one.3. Full render through the fixed pipeline — a real 24 fps mp4 with audio out of the producer: video
start_time=0.000000, audiostart_time=0.000000, one edit per track (media time: 0), no empty edit.4. Unit + static checks (ffmpeg 8.1.2, bun 1.3.14, macOS):
MuxVideoWithAudioOptions.Investigated, written, and tested with Claude Code (Claude Fable 5).